Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: SDK reports OOM when crashing after calling close #2468

Merged
merged 5 commits into from
Nov 30, 2022
Merged

fix: SDK reports OOM when crashing after calling close #2468

merged 5 commits into from
Nov 30, 2022

Conversation

kevinrenskers
Copy link
Contributor

@kevinrenskers kevinrenskers commented Nov 30, 2022 •
edited
Loading

📜 Description

The app state has a new property isSDKRunning which gets set to false when the SDK closes. This property is then used in the OOM logic, returning false when isSDKRunning is false.

💡 Motivation and Context

Closes #2457

💚 How did you test it?

Unit tests, and steps as described in #2457.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link

github-actions bot commented Nov 30, 2022 •
edited
Loading

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1250.94 ms 1257.64 ms 6.70 ms
Size 20.75 KiB 383.38 KiB 362.63 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
68094b3 1214.14 ms 1255.09 ms 40.95 ms
3fdb749 1227.42 ms 1248.48 ms 21.06 ms
d10145a 1232.65 ms 1257.55 ms 24.90 ms
9f8d429 1239.57 ms 1255.12 ms 15.55 ms
e2cec76 1189.48 ms 1229.84 ms 40.36 ms
fb9f27a 1218.55 ms 1249.17 ms 30.62 ms
a9db8a6 1199.83 ms 1234.56 ms 34.73 ms
9cb6c52 1201.08 ms 1211.37 ms 10.29 ms
dcac8ad 1238.82 ms 1247.80 ms 8.98 ms
1ce879f 1258.12 ms 1260.90 ms 2.78 ms

App size

Revision Plain With Sentry Diff
68094b3 20.75 KiB 373.94 KiB 353.19 KiB
3fdb749 20.75 KiB 383.81 KiB 363.06 KiB
d10145a 20.75 KiB 379.12 KiB 358.36 KiB
9f8d429 20.75 KiB 383.40 KiB 362.65 KiB
e2cec76 20.75 KiB 381.81 KiB 361.06 KiB
fb9f27a 20.75 KiB 382.11 KiB 361.36 KiB
a9db8a6 20.75 KiB 383.37 KiB 362.62 KiB
9cb6c52 20.75 KiB 382.12 KiB 361.36 KiB
dcac8ad 20.75 KiB 379.11 KiB 358.36 KiB
1ce879f 20.75 KiB 381.81 KiB 361.06 KiB

Previous results on branch: fix/2457-OOM-after-close

Startup times

Revision Plain With Sentry Diff
19d4324 1217.57 ms 1243.02 ms 25.45 ms
bbe9a91 1229.46 ms 1243.50 ms 14.04 ms

App size

Revision Plain With Sentry Diff
19d4324 20.75 KiB 383.38 KiB 362.62 KiB
bbe9a91 20.75 KiB 383.38 KiB 362.63 KiB

brustolin
brustolin approved these changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevinrenskers kevinrenskers merged commit c7d7a55 into 8.0.0 Nov 30, 2022
@kevinrenskers kevinrenskers deleted the fix/2457-OOM-after-close branch November 30, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brustolin brustolin brustolin approved these changes

@philipphofmann philipphofmann Awaiting requested review from philipphofmann philipphofmann is a code owner

@armcknight armcknight Awaiting requested review from armcknight armcknight is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kevinrenskers @brustolin